Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Table_Tests to the builder API #8622

Merged
merged 98 commits into from
Jan 26, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Dec 21, 2023

Closes #7566

Pull Request Description

Refactor test/Table_Test to the builder API. The builder API is in a new library called Test_New that is alongside the old Test library. There will be follow-up PRs that will migrate the rest of the tests. Meanwhile, let's keep these two libraries, and merge them after the last PR.

Important Notes

  • For a brief introduction into the new API, see Prototype 1 section in Refactor Table_Tests to the builder API #8622 (comment)
  • When executing all the tests, the behavior should be the same as with the old library. With the only exception that if ENSO_TEST_ANSI_COLORS env var is set, the output is more colorful than it used to be.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Compare the JUnit.xml output with the old library.
    • Make sure that all the old tests are run.
  • Compare the total run time of enso --no-ir-caches --run test/Table_Tests.
    • Ensure that there is no major regression
  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.

@Akirathan Akirathan self-assigned this Dec 21, 2023
@Akirathan
Copy link
Member Author

Akirathan commented Dec 27, 2023

Proposition (1)

Let's try to retain the old API as much as possible. The standalone (only in one source) prototype is in test/Tests/src/Proposition_1.enso. In this prototype:

  • All the test groups and specs are first collected, and then run.
  • The collection of tests is done in Test_Suite.collect method.
    • We use State of Test_Suite_Builder and Group_Builder to keep track of groups and suites that are being collected. Both of these are just Vector builders.
  • The API is the same as it used to be - we only need to add the Test_Suite.collect method call to every test source file.
  • The test data initialization (setup) can be done via lazy atom fields.
  • This concept can be expanded to multiple sources just by merging all the Test_Suite from all the sources.
    • We can do some validation there, like ensure that group names do not clash.

The output of running the prototype with enso --run test/Tests/src/Proposition_1.enso is:

All test names:
  my-group:my-spec-1
  my-group:my-spec-2
  my-group-2:my-spec-3
Running tests...
Running group 'my-group'
  Running spec 'my-spec-1'
    In my-spec-1
  Running spec 'my-spec-2'
    In my-spec-2
Creating test data
    my-spec-2: res = True
Running group 'my-group-2'
  Running spec 'my-spec-3'
    In my-spec-3
Done running tests.

From the output, we can see that the concept of first collecting and then running the tests works.

The advantage of this proposition is that the vast majority of test sources will stay the same, the disadvantage is that we have to use State to keep track of the builders and that the test sources will not look like the bench sources (for example test/Benchmarks/src/Equality.enso)

Proposition (2)

The proposition (2) introduces a similar builder API pattern that was introduced to benchmarks in #7324. Its prototype is implemented as Test_New library, the main source so far is Test_New/src/Test_New.enso. Usage of this builder API is demonstrated in test/Tests/src/Data/Bool_Spec_New.enso.

The output of running enso --in-project test/Tests --run test/Tests/src/Data/Bool_Spec_New.enso (excluding unused variable binding warnings) is:

Running `Test.build` (collecting tests)...
Done with `Test.build` (collecting tests)
Running `Test.run_main` - running all tests
Running group 'Booleans'
  Running spec 'should allow converting Bools to Text values'
    In First spec
  Running spec 'should allow for comparing Bools'
    In Second spec
Done with `Test.run_main` - running all tests

Conclusion

So which proposition do you like better? (1) or (2)? cc @JaroslavTulach @hubertp @4e6 @jdunkerley @radeusgd @GregoryTravis

@JaroslavTulach
Copy link
Member

The closer to Bench API the better.

Ideally I'd like to see just a single API for building group of specs of Tests and Benchmarks. There is no difference between building a test and building a benchmark as far as I can tell. Running tests and running benchmarks is different. However building tests and benchmarks is the same.

On the other hand, searching for the proper abstraction may be too much for now, so: To keep things simple, we can copy & paste. In such case, please mimic:

type Bench
    ## A set of groups of benchmarks.
    All (groups : Vector Bench)

    ## A single group of benchmarks sharing configuration.
    Group (name:Text) (configuration:Bench_Options) (specs : Vector Bench)

    ## A specific single benchmark.
    Spec (name:Text) (code : Any -> Any)

    ## Construct a Bench object.
    build : (Bench_Builder -> Any) -> Bench
    build fn =
        b = Vector.new_builder
        fn (Bench_Builder.Impl b)
        groups_vec = b.to_vector
        Bench.All groups_vec . validate

moreover, if we 100% copy - we may find the proper abstraction between tests and benchmarks later. As such my choice is clear:

The closer to Bench API the better.

@radeusgd
Copy link
Member

radeusgd commented Jan 2, 2024

I agree that API closer to benchmarks is probably better.

We will have to rewrite big chunks of our testing code anyway to handle the initialization correctly, so changing the syntax slightly is not the biggest pain anyway.

With that, I think that we may need some more involved setup/teardown support than just the lazy-fields trick that we used in benchmarks. The benchmarks are mostly relying on 'referentially transparent' initialization that is lazy only because it is costly (e.g. constructing a large vector / table).

In our test suite, the initialization tends to be more involved, like setting up Database connections and creating temporary tables; creating temporary files that often need to be cleaned up afterwards. I'm not really sure that the lazy-field pattern will adapt well to that - we actually have a lot of this kind of state scattered in our tests (probably because this played well with the current test API).

I think we should try adapting one of the more complicated tests and see how the possible API (be it the variant 1 or 2) plays with that, and if the lazy-fields are a good solution or if we need something else for that.

I think the SQLite_Spec.enso is a good 'benchmark' of the kind of 'mess' that we have with initialization of various stuff within the Test.group statements and even outside of them - all of which has to be somehow made 'lazy' to not run when only gathering the test structure.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving this forward.

@Akirathan
Copy link
Member Author

Akirathan commented Jan 24, 2024

There is one last failure in Table_Spec.enso - https://github.com/enso-org/enso/actions/runs/7639372482/job/20818708973#step:10:5104 . Seems like an error after wrong merge of develop. Let's fix that and merge this PR.

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Jan 25, 2024
@mwu-tow mwu-tow added the CI: Keep up to date Automatically update this PR to the latest develop. label Jan 26, 2024
@mwu-tow mwu-tow removed the CI: Keep up to date Automatically update this PR to the latest develop. label Jan 26, 2024
Comment on lines +31 to +32
teardown self (~code : Any) =
self.teardown_ref.put (_ -> code)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if teardown is called twice?

The old code is replaced and it will no longer be run - IMO that is not good.

Let's either:

  1. fail, letting the user know that teardown can only be called once per group
  2. append to a vector of 'finalizars' that will all be run.

Comment on lines +39 to +44
sb = StringBuilder.new
sb.append ("Group '" + self.name + "' specs=[")
self.specs.each spec->
sb.append (spec.to_text + ", ")
sb.append "]"
sb.toString
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't think we should be using Java's StringBuilder for such stuff.
This can be easily rewritten into Enso code:

Suggested change
sb = StringBuilder.new
sb.append ("Group '" + self.name + "' specs=[")
self.specs.each spec->
sb.append (spec.to_text + ", ")
sb.append "]"
sb.toString
self.specs.map .to_text . join prefix=("Group '" + self.name + "' specs=[") separator=", " suffix="]"

Comment on lines +26 to +28
run_group : Group -> Vector Test_Result
run_group (group : Group) =
run_specs_from_group group.specs group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unused. Maybe we can remove?

Comment on lines +312 to +314
# The default `connection` parameter always create a new connection.
# In some tests, for example, where we are joining tables, we have to specify
# exactly the same connection.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the multiline comment syntax

Suggested change
# The default `connection` parameter always create a new connection.
# In some tests, for example, where we are joining tables, we have to specify
# exactly the same connection.
## The default `connection` parameter always creates a new connection.
In some tests, for example, where we are joining tables, we have to specify
exactly the same connection.

Comment on lines +359 to +361
transient_dir = enso_project.data / "transient"
assert transient_dir.exists ("There should be .gitignore file in the " + transient_dir.path + " directory")
transient_dir / "sqlite_test.db"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert message is only vaguely connected to the condition it is checking, that is quite confusing for me.

Can we align this?

The assert does not check for a .gitignore file, but for the directory existing.
I know the existence of .gitignore will imply existence of the parent dir, but the converse is not true. Let's either explicitly check for .gitignore OR reword the condition. Otherwise it is just confusing.

Comment on lines +393 to +395
sqlite_spec suite_builder in_file_prefix (_ -> create_file_connection backing_file)
Transaction_Spec.add_specs suite_builder (_ -> create_file_connection backing_file) in_file_prefix
Upload_Spec.add_specs suite_builder (_ -> create_file_connection backing_file) in_file_prefix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, connecting creates the 'Dummy' table everywhere but this table was only supposed to be there for the SQLite_Format should allow connecting to SQLite files tests.

Can we restructure it to only create this table in that case?

@mergify mergify bot merged commit d0fdeca into develop Jan 26, 2024
26 of 27 checks passed
@mergify mergify bot deleted the wip/akirathan/7566-stdlib-test-builder-api branch January 26, 2024 12:08
Comment on lines +1030 to +1033
main =
suite = Test.build suite_builder->
add_specs suite_builder
suite.run_with_filter spec_filter="should write a table to non-existent file as a new sheet with headers; and return the file object on success"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this filter should not be left over in the repo, right?

@Akirathan can you please remove it in a next PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Standard.Test to the builder API
5 participants